HYPERFLEET-971 - feat: reject nodepool create/patch on soft-deleted cluster #113
HYPERFLEET-971 - feat: reject nodepool create/patch on soft-deleted cluster #113kuudori wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds a new RFC 9457 conflict error code Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as ClusterHandler / NodePoolsHandler
participant Service as ClusterService
participant Errors as errors.Registry
Client->>Handler: HTTP PATCH/POST (cluster/nodepool)
Handler->>Service: Get(clusterID)
Service-->>Handler: cluster (maybe nil)
alt cluster missing
Handler-->>Client: 404 Not Found
else cluster present
alt cluster.DeletedTime != nil
Handler->>Errors: ConflictState("marked for deletion", ...)
Errors-->>Handler: ProblemDetails (409, HYPERFLEET-CNF-003)
Handler-->>Client: 409 Conflict (problem-details)
else cluster active
opt nodepool flow: check nodepool.DeletedTime
alt nodepool.DeletedTime != nil
Handler->>Errors: ConflictState("marked for deletion", ...)
Errors-->>Handler: ProblemDetails (409,...)
Handler-->>Client: 409 Conflict
else nodepool active
Handler->>Service: Replace/Create(...)
Service-->>Handler: updated resource
Handler-->>Client: 200/201 with body
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
accidentally clicked generate unit tests under the coderabbit comment, deleted his comments with an unsuccessful attempt to do that |
|
Nice work! The soft-delete guards on the cluster-scoped endpoints look solid. One thing I noticed: the standalone Worth adding the same guard there for consistency — or at least tracking it as a follow-up. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/handlers/cluster_nodepools.go`:
- Around line 193-200: The soft-delete check on cluster.DeletedTime in the
handler (after clusterService.Get) is racy; move this validation into the
service/DAO layer and enforce it atomically with the write. Update the nodepool
creation/replacement paths (the service methods that call Create()/Replace() on
the DAO) to either re-check DeletedTime inside the same transaction or add a
precondition to the DB write (e.g., include WHERE cluster.deleted_time IS NULL
in the INSERT/UPDATE SQL) so Create()/Replace() fail if the cluster is marked
deleted; ensure the service/DAO returns a clear ConflictState error when that
condition is hit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9ca03664-3434-4b51-9885-8613945b3352
📒 Files selected for processing (5)
pkg/errors/errors.gopkg/handlers/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_nodepools_test.gopkg/handlers/cluster_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/errors/errors.go
- pkg/handlers/cluster.go
|
/retest |
| return nil, err | ||
| } | ||
|
|
||
| if cluster.DeletedTime != nil { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: JIRA
The JIRA ticket HYPERFLEET-971 acceptance criteria says "PUT /clusters/:clusterID/nodepools/:nodepoolID returns 409" but the API only exposes PATCH for nodepool updates — no PUT endpoint exists. The implementation here is correct (guarding PATCH), but worth updating the JIRA AC to match reality so the ticket closes cleanly.
| }, | ||
| CodeConflictState: { | ||
| ErrorTypeConflict, "State Conflict", | ||
| "Operation not allowed in current resource state", http.StatusConflict, |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Standards
The error model standard defines HYPERFLEET-CNF-003 as "Operation not allowed in current state" but the implementation here says "Operation not allowed in current resource state". In practice it doesn't matter since ConflictState() calls always pass a custom reason, but matching the standard keeps things consistent.
CodeConflictState: {
ErrorTypeConflict, "State Conflict",
"Operation not allowed in current state", http.StatusConflict,
},
| } | ||
| } | ||
|
|
||
| func TestClusterNodePoolsHandler_Patch(t *testing.T) { |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
The new TestClusterNodePoolsHandler_Patch covers the soft-delete scenarios well, but there's no test case for nodepool-not-found (404 from nodePoolService.Get) or nodepool-belongs-to-different-cluster (the OwnerID != clusterID guard). Since this is establishing the test suite for the Patch handler from scratch, consider adding those for completeness — or as a follow-up.
|
Nice work on the soft-delete guards! One thing to consider as a follow-up: The status endpoints ( |
2 participants
Prevent nodepool creation and updates on clusters marked for deletion by returning 409 Conflict, avoiding orphaned resources after cluster cleanup.
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit
Bug Fixes
New
Tests